We should consider method Session.destroy() to remove pages when session expires. See my comment at https://issues.apache.org/jira/browse/WICKET-6465

When the session expires or is invalidated via API call then #valueUnbound() is called and this clears the storages.
This is how it worked last time I worked in this area of the code.

Indeed, Sesison.destroy() is only called under certain conditions, and in those conditions, valueUnbound() will also be called.

I see. thank you for the clarification. Still, I'd like to evaluate more structural solutions, at least for Wicket 8. A good start point might be HttpSessionStore.SessionBindingListener#valueUnbound.
It's already used to call Session#onInvalidate and we can add Session#clear() after it.

Currently HttpSessionStore and PageStoreManager each use their own instance of HttpSessionBindingListener.
Yes, we might be able to unify these. But with a simple fix we don't introduce even more - possibly faulty - changes.

It was unfortunate that we didn't consider all possible callbacks from Servlet containers when this problem started with WICKET-6356.
IMHO we should go with the simplest fix: Prevent anything bad from happening, when re-setting the session attribute:

• clustering will work (because the attribute is re-set)
• it doesn't matter whether the container calls valueBound() or not
• if the container calls valueUnbound(), it does nothing bad during storeTouchedPages()

Andrea, what do you think?

BTW do we have to take care of concurrent access to the SessionEntry? I see that Martin used an AtomicBoolean, but how does that help if there are two request simultaneously storing touched pages?

I'm fine with this quick fix, but I warmly suggest to improve it after it has been applied, at least in master branch.

About concurrent access afaik we do have to take care of concurrency. At the moment we don't have any guarantee that setSessionAttribute saves the entry for the last request submitted (for the same session).

I don't see why the current implementation uses AtomicBoolean. The current implementation could just as well use a normal boolean. If we need to take concurrent access into account (and I think we do), we should probably use a ThreadLocal. Of course this assumes that the servlet container will call (un)bound from the same thread, but the current implementation already has that assumption. If a container decides to make the calls async, there is no way of telling if it will happen between the setting and clearing of the boolean.

I agree about the use of a normal boolean, but I'm not sure I've understood your idea about ThreadLocal. What I would like to do is to prevent race conditions inside storeTouchedPages. I would do it using synchronized on entry object:

protected void storeTouchedPages(final List<IManageablePage> touchedPages)
{
	if (!touchedPages.isEmpty())
	{
		SessionEntry entry = getSessionEntry(true);
		synchronized (entry) 
		{
			entry.setSessionCache(touchedPages);
			for (IManageablePage page : touchedPages) 
			{
				// WICKET-5103 use the same sessionId as used in SessionEntry#getPage()
				pageStore.storePage(entry.sessionId, page);
			}
			entry.updating.set(true);
			setSessionAttribute(getAttributeName(), entry);
		}
	}
}

In this way two possible concurrent requests for the same session would execute storeTouchedPages one at a time.

We don't care about concurrent calls to valueUnbound, that's fine. All code in that method is thread safe. What we don't want is multiple calls to storeTouchedPages and valueUnbound (session expiry) to interfere with each other. Synchronizing on the entry will not help as session expiry is probably done from a different thread.

We don't care about concurrent calls to valueUnbound, that's fine. All code in that method is thread safe.

I agree

What we don't want is multiple calls to storeTouchedPages and valueUnbound (session expiry) to interfere with each other. Synchronizing on the entry will not help as session expiry is probably done from a different thread.

Yes, synchronizing is meant to prevent problems for multiple invocations of storeTouchedPages for the same session, valueUnbound is unaffected. The boolean value should be enough to tell if valueUnbound is invoked as result of session expiration.

No, a boolean won't work. Suppose the following happens: A thread calls storeTouchedPages, setting the boolean to true. At the same time, another thread unbinds the session, calling valueUnbound. This will leak the page store. We only want valueUnbound to be ignored when its a side effect of calling storeTouchedPages. This is local to the thread calling the method, hence ThreadLocal.

Got it. That's way I hope for a future improvement of this boolean-based solution :-).

Agreed, a ThreadLocal makes sense.

Yes, this is what I had in mind. I'm ok to merge this in 7.x and 8.

Don't forget to fix brackets indentation before commit